Skip to content

Build dockers #1298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Build dockers #1298

wants to merge 1 commit into from

Conversation

rbanka1
Copy link
Contributor

@rbanka1 rbanka1 commented May 6, 2025

Added UMF docker

This is the first part only for docker build. The second with updated CI : #1329

// workflow executed on fork:
https://github.com/rbanka1/unified-memory-framework/actions/runs/15134054789
https://github.com/rbanka1/unified-memory-framework/actions/runs/15134054814

I added a new Docker image for Ubuntu 24.04 and updated the others.
I also created two new workflows that check for changes in the Docker files.
If any changes are detected, the relevant Docker image will be rebuilt and pushed (unless it's a pull request).
The rebuilt images will be available after the merge.

@KFilipek
Copy link
Contributor

KFilipek commented May 7, 2025

@lukaszstolarczuk
We have https://github.com/bb-umf account on GitHub.

@lukaszstolarczuk lukaszstolarczuk mentioned this pull request May 8, 2025
10 tasks
@rbanka1 rbanka1 force-pushed the newDocker branch 2 times, most recently from 6039c67 to b1d2ec2 Compare May 13, 2025 11:58
@@ -192,7 +166,6 @@ jobs:
-DUMF_DISABLE_HWLOC=${{matrix.disable_hwloc}}
-DUMF_LINK_HWLOC_STATICALLY=${{matrix.link_hwloc_statically}}
${{ matrix.build_type == 'Debug' && matrix.compiler.c == 'gcc' && '-DUMF_USE_COVERAGE=ON' || '' }}
${{ matrix.llvm_linker || '' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this is removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returned

@rbanka1 rbanka1 changed the title New docker UMF Add dockers to Fast and Basic Builds May 15, 2025
@rbanka1 rbanka1 force-pushed the newDocker branch 5 times, most recently from 3552ecc to aefd38a Compare May 20, 2025 10:43

jobs:
CodeChecks:
uses: ./.github/workflows/reusable_checks.yml
FastBuild:
name: Fast builds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the name was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returned

run: |
echo "Changed files: $ALL_CHANGED_FILES"

RunReusableDocker:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps simply BuildDocker

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

strategy:
matrix:
os: ['ubuntu-22.04', 'ubuntu-24.04']
ubuntu_ver: ['ubuntu-22.04', 'ubuntu-24.04']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably go back to the os name, as it contains not only the ubu ver

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

- name: Install g++-7
if: matrix.compiler.cxx == 'g++-7'
run: sudo apt-get install -y ${{matrix.compiler.cxx}}
echo "${USERPASS}" | sudo -Sk bash -c "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some as above, perhaps sudo su

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote installation script to avoid multi-line bash command

runs-on: ubuntu-latest
strategy:
matrix:
ubuntu-version: [20.04, 22.04, 24.04]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe for unity, this should also be os (full name of the distro)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

strategy:
matrix:
include:
- os: windows-latest
- ubuntu_ver: ubuntu-22.04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same - re-name to os please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

build-essential \
cmake \
git \
gnupg"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it should be moved to oneapi installation step

(applies to both docker files)

${{ matrix.install_tbb == 'ON' && matrix.disable_hwloc != 'ON' && matrix.shared_library == 'ON' && '--proxy' || '' }}
--umf-version ${{env.UMF_VERSION}}
${{ matrix.shared_library == 'ON' && '--shared-library' || '' }}
# - name: Test UMF installation and uninstallation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and I believe it's the time to uncomment this - what's failing with that? please send me a link and we'll investigate...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary, it is working :)

@rbanka1 rbanka1 force-pushed the newDocker branch 4 times, most recently from f15cda9 to 5705c2a Compare May 22, 2025 14:01
Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the CI is failing unfortunately - is it possible that it needs some of the fixes in docker that wasn't pushed yet?

perhaps you should split your PR into 2 - one with docker changes, and the other with the rest...?

on:
pull_request:
paths:
- '.github/docker/*.Dockerfile'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.github/scripts dir should be added as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not done yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? I added the path

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where? It is added on push, but on pull_request it is missing:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry, now it is done

@@ -28,7 +35,7 @@ jobs:
link_hwloc_statically: ['OFF']
cmake_ver: ['default']
include:
- os: 'ubuntu-22.04'
- os: ubuntu-22.04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sry for being meticulous, but you could leave ' as they were here before... we'll potentially have less conflicts on any merges, and less changes here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@rbanka1 rbanka1 changed the title Add dockers to Fast and Basic Builds Build dockers May 23, 2025
@rbanka1 rbanka1 marked this pull request as ready for review May 23, 2025 20:32
@rbanka1 rbanka1 requested a review from a team as a code owner May 23, 2025 20:32
CONTRIBUTING.md Outdated
If you want to rebuild existing Docker images or add a new one, you must open a separate pull
request dedicated to Docker-related changes. This PR must be merged into the main branch first.

Only after the PR is merged will the updated Docker images be available for use in workflows.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The updated Docker images will be available for use in workflows only when this PR is merged."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced

I added a new Docker image for Ubuntu 24.04 and updated the others.
I also created two new workflows that check for changes in the Docker files.
If any changes are detected, the relevant Docker image will be rebuilt and pushed (unless it's a pull request).
The rebuilt images will be available after the merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants